-
-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open external links in new tabs, and warn the user #862
Conversation
I still need to add a setting to optionally disable this behavior, as we decided in #404 |
www/js/app.js
Outdated
// We also do it for ftp even if it's not supported any more by recent browsers... | ||
if (/^(?:http|ftp)/i.test(href)) { | ||
var target = e.target.getAttribute('target'); | ||
// We don't override the default browser behavior if there's already a target attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not sure about this. If I ask to be warned before the browser opens a link on the Internet (albeit in a new tab), I would expect to be warned uniformly in all cases, not only if the link happens not to have a target. So I think the warning should happen anyway, just don't set the target if it is already set. What do you think @mossroy ?
I have tested the functionality, and it works well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target , the target can be used to communicate between tabs/windows/frames/iframes etc
I thought it might be safer to not do anything in such cases.
But you're right: we're talking about the case of an external target URL, for which it should be better to warn the user anyway.
I'll change accordingly
I've added the UI option to disable this behavior, in the "display" settings. |
You mean the warning should be on by default, right? If so, I agree. We like this app to be very cautious about contacting external servers for privacy reasons (which, for example, in a certain large country right now could even entail considerable danger to someone). So I think it's best to warn before accessing such URLs by default. If it is interfering (this is extremely unlikely with currently available ZIMs) then the user can turn it off. Would it be worth adding a line at bottom of dialogue box to say "This warning can be disabled in configuration", or is it better to let user discover the setting if it's annoying them? |
The warning is already on by default. I'll had the comment in the dialog box to let the user know he/she can change this behavior. My questioning is if it should be in "display" settings or in "expert" settings. |
Ah, so it's really just about how discoverable this is, and whether we want to discourage people from turning it off. I'm not sure it's really and "Expert Setting". I think of those as settings for the more technically minded, and I don't know if this is the case. |
I realize we in fact mix 2 different topics here:
I don't think it's a big issue anyway |
OK. I'll just add a comment to discourage people from changing the default value |
Well, they are closely related, so it's OK! My personal feeling is that it's not an expert setting. I also think that it would be trivial to add the warning also in jQuery mode, for uniformity and for the same privacy reasons. But that's up to you! |
You're right, I'm trying to do that |
I've implemented the same behavior for jQuery mode: it was indeed straightforward. To me, this PR is ready for review. |
Could it be because the click is registered on the span element, and we're not catching it in the correct bubbling phase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, but I've only tested briefly, and haven't yet tested in IE11 or IE mode with jQuery, only in Chromium.
www/js/app.js
Outdated
message += ' (in a new tab)'; | ||
} | ||
message += '<br />' + event.target.href + '<br />(this behavior can be disabled in settings if you know what you do)'; | ||
uiUtil.systemAlert(message, 'Opening external link', true).then(function (response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of <br />
, I would try wrapping each part of this message in <p>
tags. At the moment it looks crowded and a bit ugly. The URL should be on its own line, with some vertical whitespace between the URL and the surrounding messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, will use <p>
tags
www/js/app.js
Outdated
@@ -1456,6 +1463,20 @@ define(['jquery', 'zimArchiveLoader', 'uiUtil', 'settingsStore','abstractFilesys | |||
// Configure home key press to focus #prefix only if the feature is in active state | |||
if (params.useHomeKeyToFocusSearchBar) | |||
iframeArticleContent.contentWindow.addEventListener('keydown', focusPrefixOnHomeKey); | |||
if (params.openExternalLinksInNewTabs) { | |||
// Add event listener to iframe window to check for links to external resources | |||
iframeArticleContent.contentWindow.addEventListener('click', function (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in another comment, it looks like we need some logic to detect a click on an element that contains an anchor element. I think it can be caught by specifying the bubbling phase without having to add lots of new logic, but that's always been a bit obscure to me and it needs some experimentation. This seems to affect some links in Wikipedia citations, for example. If we can't catch it in a simple way, simply inspecting the innerHTML (or other content) of the trapped element for an anchor target should not be hard.
I think I've faced this issue before somewhere, and there was a way to get, using DOM methods, the final target of a click....
I managed to reproduce. Will investigate |
In the ZIM file where I reproduced the issue, it comes from the fact that there is a |
Ah, so I think there is a difference between event.target and event.currentTarget (entirely from memory, I haven't checked). Maybe you can check? |
Come to think of it event.currentTarget will probably be the iframe, right? |
I think so. In any case, I implemented a fix that seems to work well, in both SW and jQuery modes. When the click event happens, I look for the closest enclosing |
Great! Do you know about |
Actually, I did not, thanks a lot for the pointer! Our support of old browsers tend to give me bad habits to do things by hand... I think it would be a good idea to use that instead of my custom code, and add the polyfill for IE. |
The code is much simpler with |
The test-offline-files github actions did very well its job to warn me I had to add the new file in the precache list of the serviceworker 👍 |
I should be able to test with IE11. |
While testing I came across this case: As you can see, because the URL doesn't have any space/break at which to wrap, it bleeds outside of the dialogue box. I think there might be a way to make it wrap regardless of whitespace. But I don't know without checking. EDIT: to reproduce, search for "wired" in the English Wikipedia "Poetry" article. |
OK, I tried several things, I even tried a different polyfill, but none seems to work with IE11. I wonder if it's something to do with RequireJS unloading modules it thinks aren't being used. Then I remembered that jQuery has a We only need to use it in the JQuery mode (obvious, really!). I'll make some suggested changes that I've tested, and they work (on quick test) in IE11. It works with a straight anchor target, and also with the cite element that is wrapped in an anchor target that we tested above. |
OK. I'm really puzzled by why the Polyfill wouldn't run in IE11. It must be something to do with Require isolating it, though the Element prototype is patched by the Polyfill. I think using your handcrafted solution if closest isn't available is a good idea if you would like to avoid jQuery here. |
I just pushed it. |
It's pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested in Chromium (Edge) in SW and jQuery modes, and in IE11.
It worked fine on the links I tested in both browsers. I didn't test AREA tags that might have external links (I didn't find an example).
I had some very minor code suggestions you can take or leave! (They are just for clarity.)
PS Very useful PR, that makes behaviour consistent between Kiwix JS and other Kiwix apps. I'll also add it downstream when merged. |
Fixes #404 This is a refresh of the work of Jaifroid in branch https://github.com/kiwix/kiwix-js/tree/Check-for-external-links-and-warn
2bb7544
to
6bc0b93
Compare
Fixes #404
This is just a refresh of the work of Jaifroid in branch https://github.com/kiwix/kiwix-js/tree/Check-for-external-links-and-warn